Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: make the patch to objchange.ProposedNew explicit #2246

Merged
merged 1 commit into from
Jul 25, 2024
Merged

chore: make the patch to objchange.ProposedNew explicit #2246

merged 1 commit into from
Jul 25, 2024

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Jul 24, 2024 •
edited
Loading

With this change it is safe to rerun go generate to vendor objchange.ProposedNew algorithm and its modification is made
explicit by writing out the intentional patch.

Revisiting the patch is tracked in #2247

t0yv0
t0yv0 commented Jul 24, 2024
View reviewed changes
pkg/tfbridge/provider_test.go Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Jul 24, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.67%. Comparing base (00f5546) to head (0fb04a1).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2246      +/-   ##
==========================================
- Coverage   60.68%   60.67%   -0.01%     
==========================================
  Files         356      356              
  Lines       46451    46451              
==========================================
- Hits        28189    28185       -4     
- Misses      16705    16708       +3     
- Partials     1557     1558       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@t0yv0 t0yv0 requested a review from VenelinMartinov July 25, 2024 00:00
@t0yv0
Copy link
Member Author

t0yv0 commented Jul 25, 2024

Stepping through the test case. proposedNewNestingSet is actually the one introducing this empty object into the plan.

Some tracing:

=== RUN   TestPlanResourceChangeUnknowns
=== RUN   TestPlanResourceChangeUnknowns/unknown_for_set_block_prop
objchange.proposedNew 
 prior=cty.ObjectVal(map[string]cty.Value{"id":cty.NullVal(cty.String), "list_block_prop":cty.ListValEmpty(cty.Object(map[string]cty.Type{"prop":cty.String})), "list_prop":cty.NullVal(cty.List(cty.String)), "max_items_one_block_prop":cty.ListValEmpty(cty.Object(map[string]cty.Type{"prop":cty.String})), "max_items_one_prop":cty.NullVal(cty.List(cty.String)), "nested_list_block_prop":cty.ListValEmpty(cty.Object(map[string]cty.Type{"nested_prop":cty.List(cty.Object(map[string]cty.Type{"prop":cty.String}))})), "nested_list_prop":cty.NullVal(cty.List(cty.List(cty.String))), "set_block_prop":cty.SetValEmpty(cty.Object(map[string]cty.Type{"prop":cty.String})), "set_prop":cty.NullVal(cty.Set(cty.String)), "string_prop":cty.NullVal(cty.String)}) 
 config=cty.ObjectVal(map[string]cty.Value{"id":cty.NullVal(cty.String), "list_block_prop":cty.ListValEmpty(cty.Object(map[string]cty.Type{"prop":cty.String})), "list_prop":cty.NullVal(cty.List(cty.String)), "max_items_one_block_prop":cty.ListValEmpty(cty.Object(map[string]cty.Type{"prop":cty.String})), "max_items_one_prop":cty.NullVal(cty.List(cty.String)), "nested_list_block_prop":cty.ListValEmpty(cty.Object(map[string]cty.Type{"nested_prop":cty.List(cty.Object(map[string]cty.Type{"prop":cty.String}))})), "nested_list_prop":cty.NullVal(cty.List(cty.List(cty.String))), "set_block_prop":cty.SetVal([]cty.Value{cty.UnknownVal(cty.Object(map[string]cty.Type{"prop":cty.String}))}), "set_prop":cty.NullVal(cty.Set(cty.String)), "string_prop":cty.NullVal(cty.String)}) 
----
Plan priorEV cty.NullVal(cty.Object(map[string]cty.Type{"prop":cty.String}))
Plan configEV cty.UnknownVal(cty.Object(map[string]cty.Type{"prop":cty.String}))
objchange.proposedNew 
 prior=cty.ObjectVal(map[string]cty.Value{"prop":cty.NullVal(cty.String)}) 
 config=cty.UnknownVal(cty.Object(map[string]cty.Type{"prop":cty.String})) 
Planned cty.ObjectVal(map[string]cty.Value{"prop":cty.NullVal(cty.String)})
----
objchange.proposedNew 
 prior=cty.ObjectVal(map[string]cty.Value{"prop":cty.NullVal(cty.String)}) 
 config=cty.UnknownVal(cty.Object(map[string]cty.Type{"prop":cty.String})) 
proposedNewNestingSet -->  cty.SetVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"prop":cty.NullVal(cty.String)})})
--- PASS: TestPlanResourceChangeUnknowns (0.06s)
    --- PASS: TestPlanResourceChangeUnknowns/unknown_for_set_block_prop (0.06s)
PASS
ok  	github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge	0.532s

This is the line:

		newVals = append(newVals, proposedNewBlockOrObject(schema, priorEV, configEV))

it manufactures null for priorEV via this line if no matching priorEV is found:

		if priorEV == cty.NilVal {
			priorEV = cty.NullVal(config.Type().ElementType())
		}

Then it pushes down this nil to proposedNewBlockOrObject which may replace it with an empty value by:

	if prior.IsNull() {
		// In this case, we will construct a synthetic prior value that is
		// similar to the result of decoding an empty configuration block,
		// which simplifies our handling of the top-level attributes/blocks
		// below by giving us one non-null level of object to pull values from.
		//
		// "All attributes null" happens to be the definition of EmptyValue for
		// a Block, so we can just delegate to that
		prior = schema.EmptyValue()
	}

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 25, 2024

I suspect this behavior holds in TF/OpenTOFU in which case we should take matching behavior it and maybe file a fix with them?

If we insist on fixing this, I don't think the prior change was surgical enough to fix the issue. In fact it's highly suspect.

@t0yv0 t0yv0 requested a review from iwahbe July 25, 2024 00:08
VenelinMartinov
VenelinMartinov approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching!

@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Jul 25, 2024 •
edited
Loading

Note that the failing test is only under non-PRC, so we likely shouldn't over-index on it.

I take that back - some tests under PRC are failing from #2140

@t0yv0
Copy link
Member Author

t0yv0 commented Jul 25, 2024

Yes, this patch is doing good things for Pulumi. I'm wondering if we shipped this anywhere yet. It is possible that we can opt into keeping it.

@VenelinMartinov VenelinMartinov mentioned this pull request Jul 25, 2024
Merged
@t0yv0 t0yv0 force-pushed the t0yv0/restore-objchange-vendoring branch from 959b5e5 to c69fd0a Compare July 25, 2024 15:54
@t0yv0
Explicitly patch ProposedNew
0fb04a1 
With this change it is safe to rerun go generate to vendor objchange.ProposedNew algorithm and its modification is made
explicit by writing out the intentional patch.

Revisiting the patch is tracked in #2247
@t0yv0 t0yv0 force-pushed the t0yv0/restore-objchange-vendoring branch from c69fd0a to 0fb04a1 Compare July 25, 2024 16:05
@t0yv0 t0yv0 changed the title Rerun go generate chore: make the patch to objchange.ProposedNew explicit Jul 25, 2024
@t0yv0 t0yv0 enabled auto-merge (squash) July 25, 2024 16:06
@t0yv0
Copy link
Member Author

t0yv0 commented Jul 25, 2024

Decided to keep carrying the patch, documented it better and made explicit. Thanks!

@t0yv0 t0yv0 merged commit 59fe4bf into master Jul 25, 2024
11 checks passed
@t0yv0 t0yv0 deleted the t0yv0/restore-objchange-vendoring branch July 25, 2024 16:25
VenelinMartinov added a commit that referenced this pull request Jul 25, 2024
@VenelinMartinov
Terraform integration tests (#2162)
a61fbc4 
This defines a easy to use interface for writing terraform providers and
integration tests in a concise way.

It doesn't have automation API, so not as nice as the pulumi ones but I
think this is useful for checking certain TF/opentofu behaviours.

I've moved/adapted the tf driver from the cross-tests folder into a
separate module.

I used this to explore the TF behaviour around
#2246
@mjeffryes mjeffryes added this to the 0.108 milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VenelinMartinov VenelinMartinov VenelinMartinov approved these changes

@iwahbe iwahbe Awaiting requested review from iwahbe

Assignees

@t0yv0 t0yv0

Labels
None yet
Projects
None yet
Milestone
0.108
Development

Successfully merging this pull request may close these issues.
3 participants
@t0yv0 @VenelinMartinov @mjeffryes